-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add the redirectUrl argument and use it in loginUrl #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review
index.js
Outdated
buttonText = "Login", | ||
buttonClass = "", | ||
loggedInTextClass = "", | ||
) { | ||
const container = document.getElementById(containerId) | ||
const parsedBaseUrl = new URL(baseUrl) | ||
const apiUrl = `${parsedBaseUrl.origin}/api/v0/users/me?format=json` | ||
const loginUrl = `${parsedBaseUrl.origin}/login/ol-oidc/` | ||
const redirectUrlParam = | ||
redirectUrl !== "" ? `?redirect_url=${redirectUrl}` : "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be encoded:
redirectUrl !== "" ? `?redirect_url=${redirectUrl}` : "" | |
redirectUrl !== "" ? `?redirect_url=${encodeURIComponent(redirectUrl)}` : "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I implemented your suggestion
85d83b7
to
677bf66
Compare
@rhysyngsun This is ready for another look |
Note: I'm not sure if there will be docs elsewhere but the instructions were missing the step to set the environment variable |
Yea, sorry about that. @ChristopherChudzicki merged a PR into my branch for this that adds that variable and I forgot to update the testing instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What are the relevant tickets?
Closes #6
Description (What does it do?)
This PR adds a new argument to
initLoginButton
,redirectUrl
. This string argument is simply passed through as theredirect_url
query string parameter to the login endpoint. If the domain of the redirect URL is set in theSOCIAL_AUTH_ALLOWED_REDIRECT_HOSTS
setting on the instance ofmit-open
that you are hitting, then you will be redirected back to that URL after logging in.How can this be tested?
/users/me
endpoint:mit-open
. Follow the instructions in themit-open
readme for initial setup. You will need an account in your local instance for this to work. Your instance ofmit-open
should be accessible athttp://od.odl.local:8063
. You will need to set the following environment variables:ocw-hugo-themes
(https://github.com/mitodl/ocw-hugo-themes) set up locally, running a course usingyarn start course
. Follow the readme there for basic setup instructions and make sure you can start up a course. After you have that working, check out thecg/add-mit-open-login-button
branch. Assuming you have cloned this repo, runyarn add /path/to/mit-open-login-button
to add your local copy of the code for this to your locally runningocw-hugo-themes
. This is necessary because the package is not yet published to NPM. You will also need an entry in yourhosts
file that points theocw.odl.local
domain to127.0.0.1
.mit-open
mit-open
Logged in as: username
with your username in place ofusername
mit-open